Skip to content

feat: Replace jsoniter macros with circe #83

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 21 commits into from
May 13, 2025
Merged

Conversation

ghostbuster91
Copy link

@ghostbuster91 ghostbuster91 commented May 8, 2025

This change is binary incompatible due to removal of fields that used to hold jsoniter codec instances.

@ghostbuster91 ghostbuster91 requested a review from kubukoz May 8, 2025 16:36
@@ -33,7 +33,7 @@ val commonSettings = Seq(
"com.disneystreaming" %%% "weaver-cats" % "0.8.4" % Test
),
mimaPreviousArtifacts := Set(
organization.value %%% name.value % "0.0.7"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: does the bincompat breakage affect langoustine? conversely, are the codec instances even used / need to be visible outside of jsonrpclib?

Copy link
Author

@ghostbuster91 ghostbuster91 May 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good question, I checked it manually and it seems that there is one place where it does:

[error] -- [E172] Type Error: /home/kghost/workspace/langoustine/modules/tracer/frontend/src/main/scala/component.jsonviewer.scala:63:44
[error] 63 |      displayJson(ep, mode.signal, modalBus)
[error]    |                                            ^
[error]    |No given instance of type com.github.plokhotnyuk.jsoniter_scala.core.JsonValueCodec[
[error]    |  jsonrpclib.ErrorPayload] was found for a context parameter of method displayJson in package langoustine.tracer

and there is another one in tests:

[info] compiling 17 Scala sources to /home/kghost/workspace/langoustine/modules/tests/target/jvm-3/test-classes ...
[error] -- [E172] Type Error: /home/kghost/workspace/langoustine/modules/tests/src/test/scala/testkit.scala:95:60
[error] 95 |                  upickle.default.read[req.Out](writeToArray(outc.encode(res)))
[error]    |                                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[error]    |No given instance of type com.github.plokhotnyuk.jsoniter_scala.core.JsonValueCodec[jsonrpclib.Payload] was found
[error] one error found

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we make the circe codecs package private?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sigh... removing jsoniter macros was supposed to help us in the compilation flakiness 😓

not sure about circe codecs. I can see it being useful for tests (like what langoustine did), but the tracer's usecase should probably be supported with something more high-level

Comment on lines 11 to 12
def apply(a: A): Json =
documentToJson(Document.encode(a))
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

an alternative to this would be to actually have a first class support for circe in smithy4s. But this seems to be quite a big increase in terms of maintenance. Also, I am not sure if doing it 100% with circe would actually bring any performance gains as here the heavy lifting is handled by jsoniter.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as discussed in DMs I think smithy4s-circe is unmaintainable, besides it's effectively the same as our Document codecs except with a different target. Something that we'd get "for free" if we had disneystreaming/smithy4s#1659

for now I would keep the Blob / Array[Byte] as it doesn't have the same problem, and is (possibly) handled by jsoniter by copying a contiguous array chunk rather than creating an in-memory AST...

and Document.encode means we need to involve a cache, presumably the global one

Copy link
Author

@ghostbuster91 ghostbuster91 May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Baccata what do you think? Do we keep it like that with the intermediate translation via document or should it be reverted back to Array[Byte]?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, besides the fact that the schema is implicit, I'm fine with the current code living here. I don't want to maintain a smithy4s-circe module

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alright then, let's roll with this

Comment on lines +31 to +32
case PayloadPath.Segment.Label(name) => CursorOp.DownField(name)
case PayloadPath.Segment.Index(i) => CursorOp.DownN(i)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is pretty cool

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope it works :D

@ghostbuster91 ghostbuster91 merged commit 5359542 into smithy4s-integration May 13, 2025
2 checks passed
@kubukoz kubukoz deleted the circe branch May 13, 2025 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants